-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
java-baseball 김동하 과제 pr입니다! #2
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요구사항을 잘 충족한 것 같습니다! 코드리뷰를 받은 후 꾸준히 리팩토링하면 더 좋은 코드를 작성할 수 있을겁니다.
하나의 클래스는 하나의 관심사에 집중하는 것이 좋습니다. Application
클래스에서 게임관련 로직을 분리시켜보는 것부터 리팩토링을 진행해보세요 👊
if(number.length() != 3) { | ||
throw new IllegalArgumentException(); // 세자리 수 | ||
} | ||
if(number.contains("0")) { | ||
throw new IllegalArgumentException(); // 0이 있는지 | ||
} | ||
for(int i = 0; i < number.length(); i++){ | ||
if(!(number.charAt(i) > '0' && number.charAt(i) <= '9')) throw new IllegalArgumentException(); //숫자가 아닌지 | ||
} | ||
if(number.length() != 3)throw new IllegalArgumentException(); // 길이가 다른지 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
길이 검증을 2번씩 합니다.
for(int i = 0; i < number.length()-1; i++){ | ||
for(int j = i+1; j < number.length(); j++){ | ||
if(number.charAt(i) == number.charAt(j))throw new IllegalArgumentException(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
중복 여부를 확인하는 것 같은데 이런 로직은 메소드로 분리하면 더 읽기 쉬운 코드가 될 것 같습니다.
} | ||
} | ||
} | ||
public static int countStrike(String random_num, String input_num){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java에서는 기본적으로 CamelCase를 사용합니다.
for(int i = 0; i < 3; i++){ | ||
if(random_num.charAt(i) == input_num.charAt(i)) count++; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나중에 요구사항이 변경되어서 숫자가 4자리가 될 수도 있습니다. 이럴 때를 대비해서 상수값을 하드코딩 하는 것보단 다른 공간에 분리해서 참조하는 편이 낫습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드 가독성을 위해 if문 뒤에는 단순한 로직이더라도 중괄호 안에 작성해줍니다.
public static String setStr(int strike, int ball){ | ||
String str; | ||
if(strike != 0 && ball != 0){ | ||
str = ball + "볼 " + strike + "스트라이크"; | ||
return str; | ||
} | ||
else if(strike == 0 && ball != 0){ | ||
str = ball +"볼 "; | ||
return str; | ||
} | ||
else if(strike != 0 && ball == 0){ | ||
str = strike + "스트라이크 "; | ||
return str; | ||
} | ||
return "낫싱"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메소드명만으로 어떤 기능을 하는지 파악할 수 있어야합니다. setStr이라는 이름만 들었을 때 어떤 기능을 하는 메소드인지 파악하기 어렵습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스트라이크
와 같은 문자열들도 하드코딩되어 있는데 상수처럼 분리하여 참조하는 것이 나아보입니다. 요구사항 변경에 유연한 코드를 작성해봅시다.
public static void startGame(){ | ||
int strike = 0; | ||
int ball = 0; | ||
String randNum = createRandomBall(); | ||
String inputNum = ""; | ||
while(!randNum.equals(inputNum)) { | ||
inputNum = input(); | ||
checkException(inputNum); | ||
strike = countStrike(randNum, inputNum); | ||
ball = countBall(randNum, inputNum); | ||
String result = setStr(strike, ball); | ||
System.out.println(result); | ||
if (strike == 3) { | ||
System.out.println("3개의 숫자를 모두 맞히셨습니다! 게임 종료"); | ||
return; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
알고리즘 문제를 풀 때의 코딩과 실제 개발을 할 때의 코딩은 다릅니다. 변수를 미리 선언하지 말고 필요할 때 선언과 동시에 초기화해주도록합시다.
int strike = countStrike(randNum, inputNum);
int ball = countBall(randNum, inputNum);
이렇게 수정해주면 코드도 짧아지고 가독성도 향상됩니다.
public enum BallBoundary { | ||
MAX_VALUE(9),MIN_VALUE(1); | ||
private final int value; | ||
BallBoundary(int value) {this.value = value;} | ||
public int getValue(){return value;} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 enum으로 상수를 정의할 수도 있고
public static int MAX_VALUE = 9;
와 같이 Constant를 저장하는 클래스에 static 변수로 저장할 수도 있습니다.
No description provided.